Change some panics to errors in parquet decoder #8602
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rambleraptor
force-pushed
the
fix-some-panics
branch
from
17d5287 to
4d193b3
Compare
|
I guess we can lump this in with #7806 |
|
I'm happy to split this up into some separate PRs. I know it's a lot of random things as-is. |
The pedant in me wants to take you up on your offer, but there's not so much going on here that I think that's necessary. Maybe just change the title to something that sounds better 😉. ("Address panics found in external testing"). |
etseidl
approved these changes
Oct 14, 2025
There was a problem hiding this comment.
Thanks @rambleraptor, these all look sensible to me.
Would it be possible to gin up some tests for at least some of them?
scovich
reviewed
Oct 14, 2025
| return Ok((end, buf.slice(i32_size..end))); | ||
| } | ||
| } | ||
| Err(general_err!("not enough data to read levels")) |
There was a problem hiding this comment.
This is definitely an improvement over the existing code, but it opens a question:
Given that we're reading bytes from a byte buffer, it seems like we must expect to hit this situation at least occasionally? And the correct response is to fetch more bytes, not fail? Is there some mechanism for handling that higher up in the call stack? Or is there some reason it should be impossible for this code to run off the end of the buffer?
There was a problem hiding this comment.
Also -- it seems like read_num_bytes should do bounds checking internally and return Option<T>, so buffer overrun is obvious at the call site instead of a hidden panic footgun? The method has a half dozen other callers, and they all need to do manual bounds checking, in various ways and with varying degrees of safety. In particular, parquet/src/data_type.rs has two call sites that lack any visible bounds checks.
There was a problem hiding this comment.
In this particular instance we're reading a buffer that should contain an entire page of data. If it doesn't, that likely points to a problem with the metadata.
Changes to read_num_bytes would likely need more careful consideration as I suspect it might be used in some performance critical sections.
There was a problem hiding this comment.
I think changing read_num_bytes to return Option would be a good idea, as that would essentially replace the assert in that method. That assert is currently redundant if the caller already checks the bounds, so those two checks would be replaced by one against the option. In the BitReader that might actually improve performance.
rambleraptor
changed the title
alamb
changed the title
|
I took the liberty of merging up from main and fixing |
rambleraptor
force-pushed
the
fix-some-panics
branch
from
5176ace to
cdd9f0a
Compare
etseidl
reviewed
Oct 17, 2025
|
Thanks @rambleraptor. Your force push seems to have done away with the merge from head. Could you please merge in origin/main after fixing the failing tests? |
|
🤖 |
|
🤖: Benchmark completed Details
|
Co-authored-by: Ed Seidl <[email protected]>
Co-authored-by: Ryan Johnson <[email protected]>
rambleraptor
force-pushed
the
fix-some-panics
branch
from
cdd9f0a to
56f932c
Compare
|
@etseidl sorry this took a second! Verified the test fixes and redid the rebase. |
rambleraptor
force-pushed
the
fix-some-panics
branch
from
56f932c to
72e30b0
Compare
|
Thanks @rambleraptor. I want to double check the benchmark results and see if the regressions are real. |
|
🤖 |
I started my benchmark runner again -- we'll see if it is reproducable |
|
🤖: Benchmark completed Details
|
|
I think I want to start on a parquet end-to-end benchmark of some sort. The arrow_reader bench is so sensitive. On my workstation I was seeing the BinaryViewArray bench taking about 10% longer consistently across the various permutations. Then I merged the latest from main, and one bench regressed further, but the others were consistently faster than baseline. I then reverted one change in this PR...the first regression nearly went away, but the others then regressed significantly. 🤯 I think I'm convinced these changes won't have much impact on performance. |
That would be amazing - I agree the benchmarks could definitely improved. Shall I file a ticket ? |
That's ok, I can file it. |
|
Thanks again @rambleraptor and @etseidl |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
We've caused some unexpected panics from our internal testing. We've put in error checks for all of these so that they don't affect other users.
What changes are included in this PR?
Various error checks to ensure panics don't occur.
Are these changes tested?
Tests should continue to pass.
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
Existing tests should cover these changes.
Are there any user-facing changes?
None.